Refactor helper binaries to save 161MB of disk space when the agent is installed and reduce RPM by 48MB - #1454
Conversation
| var fStartUpErrorFile = flag.String("startup-error-file", "", "file to touch if agent can't start") | ||
|
|
||
| // config-translator | ||
| var fConfigTranslator = flag.Bool("config-translator", false, "run in config-translator mode") |
There was a problem hiding this comment.
I spent some time trying to reduce the copy/paste aspect. Some of these constants are defined twice. There are some things to do to clean it up but they all end up less readable and add a lot of indirection. Open to ideas tho.
If I was forced to make one change I would put the descriptions in a constant
|
This PR was marked stale due to lack of activity. |
chadpatel
changed the title
chadpatel
changed the title
|
This PR was marked stale due to lack of activity. |
|
This PR was marked stale due to lack of activity. |
6c4f2af to
64d101d
Compare
fab8494 to
cef7fa3
Compare
movence
added
the
ready for testing
|
This PR was marked stale due to lack of activity. |
movence
changed the title
1cf69d4 to
d3a80f5
Compare
okankoAMZ
left a comment
okankoAMZ
left a comment
There was a problem hiding this comment.
Why are we deleting: cmd/amazon-cloudwatch-agent-config-wizard/wizard_test.go
okankoAMZ
left a comment
okankoAMZ
left a comment
There was a problem hiding this comment.
Will do another pass but left some comments for now
it's just being moved to |
movence
changed the title
movence
changed the title
| // Check for subcommands first | ||
| if len(os.Args) > 1 { | ||
| subcommand := os.Args[1] | ||
| if subcommand == translatorflags.TranslatorCommand || subcommand == downloaderflags.Command || subcommand == wizardflags.Command { |
There was a problem hiding this comment.
nit: stylistically I'd rather use a switch statement here but I think this is perfectly fine
| strPtr := new(string) | ||
| *strPtr = flagConfig.DefaultValue |
There was a problem hiding this comment.
Why can't we just do : strPtr = &(flagConfig.DefaultValue), do specifically don't want to update flagConfig value?
We can also just do a normal string copy and set the pointer at line 62
There was a problem hiding this comment.
we want each flag to have its own mutable copy of the default value
| args := []string{command} | ||
|
|
||
| for key, value := range flags { | ||
| if *value != "" && *value != "false" { |
There was a problem hiding this comment.
could we make these true and falses const enums?
There was a problem hiding this comment.
I think this is standard boolean flag behavior and doesn't need additional complexity.
|
|
||
| agentPath, err := findAgentBinary() | ||
| if err != nil { | ||
| // Handle error appropriately |
There was a problem hiding this comment.
how are handling the error here? Just bubble it up?
fe2b9da to
887151b
Compare
dricross
left a comment
dricross
left a comment
There was a problem hiding this comment.
Left some nits/comments, but overall looks good.
|
|
||
| const ( | ||
| Command = "config-wizard" | ||
| DefaultFilePathWindowsConfiguration = "C:\\Program Files\\Amazon\\SSM\\Plugins\\awsCloudWatch\\AWS.EC2.Windows.CloudWatch.json" |
There was a problem hiding this comment.
I know this isn't new, but it's weird that this goes to C:\Program Files\Amazon\SSM
| type Flag struct { | ||
| DefaultValue string | ||
| Description string | ||
| IsBool bool | ||
| } |
There was a problem hiding this comment.
nit: looks like this can only handle Bool and String flags. How would we handle other flag types if we needed in the future?
| return "", fmt.Errorf("amazon-cloudwatch-agent binary not found at default path: %s", paths.AgentBinaryPath) | ||
| } | ||
|
|
||
| func CreateFlagSet(command string, flagConfigs map[string]Flag) (*flag.FlagSet, map[string]*string) { |
There was a problem hiding this comment.
nit: could this take in a map[string]flag.Flag instead of our own struct so it's extensible to any flag type?
| wizardflags.Command: wizard.RunWizardFromFlags, | ||
| } | ||
|
|
||
| if err := cmdwrapper.HandleSubcommand(subcommands, handlers); err != nil { |
There was a problem hiding this comment.
nit: we already did all of the work here to find the subcommand and we already know which handler and flag set to use so the HandleSubcommand function seems to duplicate a lot of work. Is it used elsewhere or could we just replace HandleSubcommand with handler[subccommand](subcommands[subcommand]flags) or something similar?
Not that important to update though since the duplicate work is not going to impact performance in any meaningful way
There was a problem hiding this comment.
Think this is a fair point. We can improve this in a separate PR as a follow up.
…-agent to save 114M on disk and 35MB on the RPM
…urther decrease deployment size
887151b to
5fa7aa6
Compare
5 participants
Revisions:
flag.flagSetwith subcommands to cleaner parameters handling without prefixes**
Description of the issue
The cloudwatch-agent has 3 helper binaries that use an excessive amount of disk
The reason is they are directly and indirectly pulling dependencies from cloudwatch-agent. To solve this problem, I updated config-downloader, config-translator and the wizard to be shims that just redirect to amazon-cloudwatch-agent binary
This works fine, the main risk is these binaries are no longer "portable", they depend on finding the path to amazon-cloudwatch-agent at runtime. I am using the same method as
start-amazon-cloudwatch-agentfor finding the pathDescription of changes
High level the approach is to maintain the same argument interface for the existing 3 commands and seamlessly move the logic in to the main binary. To do this the old commands need to keep the same args but we prefix the args when we call CWA so that there are no duplicate args.
The general approach I took was to create a new
cmdwrapperwhich offers two methods.flagAPI for pulling command line args. I considered using subcommands but things got too complicated and ugly, prefixing was simpleramazon-cloudwatch-agentand it calls it with the new flags. It remaps stdin/stdout/stderr so it appears seemless. Things like the wizard which rely on stdin still workI moved the flags in to their own separate
flagspackages which have NO dependencies (keeping the binaries small). And then the old binaries and amazon-cloudwatch-agent pull in the flags and the commands and link everything together. For the wizard I had to move a few other common constants like the inlinuxMigration.goandwindows_migration.goI added and updated unit tests wherever possible. The old translator_test.go was moved in to translatorutil_test.go as that is primarily what those tests were testing
Note: inline diff is VERY hard to follow because of all the moved code, I recommend
splitdiff or we can do a code walk-throughLicense
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
Integ test run: https://github.com/aws/amazon-cloudwatch-agent/actions/runs/15216630285
Manual testing to confirm cloudwatch agent still starts/loads
There were no prior tests that actually tested
config-translatebinary... the old tests basically just testedcmdutilso I moved those in to thecmdutiltests. We could write more tests for the shim and fortranslate.go. Tricky tests to write due to the OS coupling.Before:
After:
Wizard still works:
Downloader/Translator still work
Rough Edges
This is the flow if config-translator fails. I tried to unify the code so behavior is slightly different. We could change this if we needed to
new:
Requirements
Before commit the code, please do the following steps.
make fmtandmake fmt-shmake lint